charts/pull-secret/README.md (2)

62-68: Consider documenting the helm-git plugin requirement.

The git+https://...@path?ref=branch repository format requires the helm-git plugin. Users integrating this as an umbrella chart dependency should be aware they need this plugin installed.

---

85-100: Configuration table is incomplete.

The table omits several parameters defined in values.yaml:

• job.backoffLimit, job.ttlSecondsAfterFinished, job.restartPolicy
• hyperfleetApi.baseUrl, hyperfleetApi.version
• image.pullPolicy, imagePullSecrets
• nodeSelector, tolerations, affinity

Consider adding these for completeness, or add a note that only commonly-customized parameters are documented here.

charts/pull-secret/values.yaml (1)

25-29: Consider using a pinned image tag for production deployments.

Using latest as the default tag is acceptable for development, but for production deployments, a specific version tag (e.g., semantic version) provides better reproducibility and rollback capability. The comment or documentation could note this recommendation.

charts/pull-secret/templates/_helpers.tpl (1)

58-64: Require explicit ServiceAccount name when binding ClusterRole.

When serviceAccount.create=false and rbac.create=true, the helper returns "default", causing the ClusterRoleBinding to grant cluster-wide secret permissions to the namespace's default ServiceAccount. This is a privilege escalation risk.

Require an explicit serviceAccount.name in this scenario.

Suggested fix

 {{- define "pull-secret.serviceAccountName" -}}
 {{- if .Values.serviceAccount.create }}
 {{- default (include "pull-secret.fullname" .) .Values.serviceAccount.name }}
+{{- else if .Values.rbac.create }}
+{{- required "serviceAccount.name must be set when serviceAccount.create=false and rbac.create=true" .Values.serviceAccount.name }}
 {{- else }}
 {{- default "default" .Values.serviceAccount.name }}
 {{- end }}
 {{- end }}

charts/pull-secret/templates/NOTES.txt (1)

35-35: Handle missing ttlSecondsAfterFinished gracefully.

If .Values.job.ttlSecondsAfterFinished is not set, this line will render as "cleaned up seconds after completion," which could confuse users.

♻️ Proposed fix to handle missing value

-The job will be automatically cleaned up {{ .Values.job.ttlSecondsAfterFinished }} seconds after completion.
+{{- if .Values.job.ttlSecondsAfterFinished }}
+The job will be automatically cleaned up {{ .Values.job.ttlSecondsAfterFinished }} seconds after completion.
+{{- else }}
+The job does not have TTL configured and will not be automatically cleaned up.
+{{- end }}

charts/pull-secret/README.md (1)

134-135: Clarify required field and auto-generation logic.

Consider enhancing the documentation:

• pullSecret.name: Clarify the auto-generation format is hyperfleet-{cluster.id}-pull-secret
• pullSecret.data: Mark explicitly as required since without it the job won't have credentials to store

Suggested documentation update

-| `pullSecret.name` | Secret name in GCP Secret Manager | Auto-generated |
-| `pullSecret.data` | Pull secret JSON data (required) | `""` |
+| `pullSecret.name` | Secret name in GCP Secret Manager | `hyperfleet-{cluster.id}-pull-secret` |
+| `pullSecret.data` | Pull secret JSON data (**required**) | `""` |

charts/pull-secret/templates/job.yaml (1)

36-42: Consider validating required environment variables.

GCP_PROJECT_ID and CLUSTER_ID are rendered even if empty, which may cause the job to fail at runtime with unclear errors. Consider using Helm's required function to fail fast during template rendering.

Suggested validation

          env:
            - name: GCP_PROJECT_ID
-              value: {{ .Values.gcp.projectId | quote }}
+              value: {{ required "gcp.projectId is required" .Values.gcp.projectId | quote }}
            - name: CLUSTER_ID
-              value: {{ .Values.cluster.id | quote }}
+              value: {{ required "cluster.id is required" .Values.cluster.id | quote }}
            - name: SECRET_NAME
              value: {{ include "pull-secret.secretName" . | quote }}

charts/pull-secret/values.yaml (1)

25-31: Avoid latest + Always for reproducible rollouts.
Consider pinning the tag and defaulting to IfNotPresent so deployments are deterministic.

Suggested values tweak

 image:
   registry: quay.io/openshift-hyperfleet
   repository: pull-secret
-  tag: "latest"
-  pullPolicy: Always
+  # Prefer a pinned tag; if templates default empty to Chart.AppVersion, keep this empty.
+  tag: ""
+  pullPolicy: IfNotPresent

charts/pull-secret/README.md (1)

43-46: Document a safer way to pass pullSecret.data.

Inline --set pullSecret.data='{"auths":...}' and embedding raw JSON in values can leak secrets to shell history and Git. Consider recommending --set-file (or an external secret) in the docs.

Proposed doc tweak

-  --set pullSecret.data='{"auths":{...}}' \
+  --set-file pullSecret.data=./pull-secret.json \

Also applies to: 60-63

charts/pull-secret/README.md (1)

142-142: Clarify the default value syntax.

The default value hyperfleet-{cluster.id}-pull-secret uses curly braces which could be ambiguous. Consider using one of these clearer alternatives:

• hyperfleet-<cluster.id>-pull-secret (angle brackets indicate placeholder)
• Auto-generated: hyperfleet-CLUSTER_ID-pull-secret
• Or add a note: hyperfleet-{cluster.id}-pull-secret (auto-generated)

📝 Suggested clarification

-| `pullSecret.name` | Secret name in GCP Secret Manager | `hyperfleet-{cluster.id}-pull-secret` |
+| `pullSecret.name` | Secret name in GCP Secret Manager | `hyperfleet-<cluster.id>-pull-secret` (auto-generated) |

charts/pull-secret/templates/rbac.yaml (1)

56-59: Prevent ClusterRoleBinding from defaulting to the namespace default ServiceAccount.

Line 58 relies on pull-secret.serviceAccountName. If rbac.create=true and serviceAccount.create=false without an explicit serviceAccount.name, this can bind cluster-wide permissions to the default SA. Please ensure the helper fails or requires an explicit name in that case.

#!/bin/bash
# Verify helper logic for pull-secret.serviceAccountName
rg -n -C4 'define "pull-secret\.serviceAccountName"' charts/pull-secret/templates/_helpers.tpl

charts/pull-secret/README.md (1)

117-123: Document the helm-git plugin requirement for umbrella chart integration.

The git+https:// repository format requires the helm-git plugin to be installed. Users attempting to use this integration will encounter errors without proper guidance.

Add to Prerequisites:

5. **helm-git plugin** (required for umbrella chart `git+` dependencies)
   ```bash
   helm plugin install https://github.com/aslafy-z/helm-git

Alternatively, consider publishing to a standard Helm repository or OCI registry to avoid this dependency.

</blockquote></details>

</blockquote></details>

<details>
<summary>🧹 Nitpick comments (6)</summary><blockquote>

<details>
<summary>charts/pull-secret/templates/NOTES.txt (1)</summary><blockquote>

`44-49`: **Keep NOTES image output aligned with resolved image logic.**

Line 45 prints `.Values.image.*` directly; if global overrides or helper-based image resolution are used elsewhere, NOTES may show a different image than what is actually deployed. Consider reusing the same helper as the deployment template here.

</blockquote></details>
<details>
<summary>charts/pull-secret/configs/pull-secret-adapter.yaml (1)</summary><blockquote>

`52-62`: **Avoid `:latest` defaults for adapter images.**

Lines 55 and 61 default to `:latest`, which makes deployments non-reproducible and can introduce unexpected changes. Prefer pinned tags or digests (even if still overridable via env/values).

</blockquote></details>
<details>
<summary>charts/pull-secret/templates/deployment.yaml (1)</summary><blockquote>

`96-102`: **Make resource limits/requests configurable via values.**

Resources are hardcoded, which limits flexibility for different environments. Users may need to tune resources based on message volume and cluster size.

<details>
<summary>♻️ Suggested changes</summary>

In `deployment.yaml`:
```diff
          resources:
-            limits:
-              cpu: 500m
-              memory: 512Mi
-            requests:
-              cpu: 100m
-              memory: 128Mi
+            {{- toYaml .Values.resources | nindent 12 }}

In values.yaml, add under a new Resources section:

# =============================================================================
# Resource Configuration
# =============================================================================
resources:
  limits:
    cpu: 500m
    memory: 512Mi
  requests:
    cpu: 100m
    memory: 128Mi

7-24: Add language specifier to fenced code block.

The architecture diagram code block should have a language specifier for accessibility and consistent rendering.

♻️ Suggested fix

-```
+```text
 ┌──────────────────────────────────────────────────────────────────┐

116-117: Use integer type for maxWaitTimeSeconds.

The value is currently a string ("300") but represents a numeric timeout. Using an integer would be more semantically correct and avoid potential type coercion issues.

♻️ Suggested fix

   # Maximum time to wait for job completion (seconds)
-  maxWaitTimeSeconds: "300"
+  maxWaitTimeSeconds: 300

---

71-89: Add validation for required broker configuration fields.

The broker configuration values (googlepubsub.projectId, googlepubsub.topic, googlepubsub.subscription for Google Pub/Sub, and rabbitmq.url for RabbitMQ) are rendered directly into the ConfigMap and environment variables without validation. Currently, users can deploy with empty values that will fail at runtime.

Add Helm's required function in configmap-broker.yaml and deployment.yaml where these values are rendered (e.g., {{ required "projectId is required when broker.type is googlepubsub" .Values.broker.googlepubsub.projectId | quote }}), or create a values.schema.json for declarative validation.